Skip to content

fix: adds accessibile labels to moderation buttons#2065

Closed
thisislawatts wants to merge 4 commits intomasterfrom
fix/moderation-how-to-buttons
Closed

fix: adds accessibile labels to moderation buttons#2065
thisislawatts wants to merge 4 commits intomasterfrom
fix/moderation-how-to-buttons

Conversation

@thisislawatts
Copy link
Contributor

@thisislawatts thisislawatts commented Jan 8, 2023

PR Checklist

PR Type

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Developer experience (improves developer workflows for contributing to the project)

Description

Use new showIconOnly property and use margin-left
for consistent layout.

Screenshots/Videos

Before:

Screenshot 2023-01-08 at 14 18 52

After:

Screenshot 2023-01-08 at 14 18 35

@thisislawatts thisislawatts requested a review from a team as a code owner January 8, 2023 13:18
Use new `showIconOnly` property and use margin-left
for consistent layout.
@thisislawatts thisislawatts force-pushed the fix/moderation-how-to-buttons branch 18 times, most recently from dd44630 to bf8e0c7 Compare January 12, 2023 20:02
@thisislawatts thisislawatts force-pushed the fix/moderation-how-to-buttons branch from bf8e0c7 to 950e9be Compare January 12, 2023 20:34
Copy link
Member

@chrismclarke chrismclarke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor comment from me but otherwise happy with the code. I'll leave it to @davehakkens if there's any related design issues

npm config set package-lock false
npm i @types/node @commitlint/types @commitlint/config-conventional --force
npx commitlint --from=$(git merge-base remotes/origin/${CIRCLE_BRANCH} master) --verbose
npm ls
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit(non-blocking)
Do we still need this line?

@chrismclarke chrismclarke self-requested a review January 21, 2023 03:10
Copy link
Member

@chrismclarke chrismclarke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thisislawatts
I tried to manually remove the additional npm-ls call however when running locally it seems like the commitlint config is not working

PS C:\apps\oneArmy\community-platform> git commit -m "chore: remove ci npm ls"
⧗   input: chore: remove ci npm ls
✖   Please add rules to your `commitlint.config.js`
    - Getting started guide: https://git.io/fhHij
    - Example config: https://git.io/fhHip [empty-rules]

✖   found 1 problems, 0 warnings
ⓘ   Get help: https://github.com/conventional-changelog/commitlint/#what-is-commitlint

Would you be able to either try fixing (I'm guessing something to do with the version installed or difference with commitlint vs @commitlint/cli), or moving the config changes into a separate PR so that we can still merge the labels update?

@chrismclarke chrismclarke self-assigned this Jan 21, 2023
@chrismclarke chrismclarke mentioned this pull request Jan 21, 2023
5 tasks
@chrismclarke
Copy link
Member

chrismclarke commented Jan 21, 2023

On second thoughts I could probably try be a bit more useful (!)
I've moved the commitlint changes to #2070 and will merge the button changes here once tests pass

There seems to be a side-issue with commitlint now not happier with the description length on some of the earlier commits so I'll also try move that to #2071 to fix

@thisislawatts thisislawatts deleted the fix/moderation-how-to-buttons branch January 23, 2023 18:09
@onearmy-bot
Copy link
Collaborator

🎉 This issue has been resolved in version 1.35.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants